-
Notifications
You must be signed in to change notification settings - Fork 46.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clear canceled task node early #16403
Conversation
No significant bundle size changes to report. Generated by 🚫 dangerJS |
Note we should probably wait before merging because this may mask the bug we found in #16145. |
Why is this change needed? Seems like it would serve to cover up another bug. |
Not strictly needed, but it's kind of inefficient? This fires on every discrete event and @acdlite's PR adds more potential work to "cancel". Unless you're saying we should keep cancel close to free instead. |
I wouldn't also say it necessarily covers things up. The way it exposed the bug was already super convoluted and hard to follow. The proper way to expose it early is by having tests for |
There are more things that are awkwardly structured with this value. What is the life-time before it gets reset to null? Seemly infinitely since it doesn't get reset when it runs. What happens if you call scheduleSyncCallback but there already is one scheduled? Then there's no way to cancel the first one. Seems like this is a leaky abstraction that has a lot of assumptions about how it's going to get called which might indicate that pretending that this is the same API as the public scheduler API isn't the right abstraction here. |
OK that is a bigger sink that I thought at first. Want to revert this? I find it confusing either way. |
Don't want to revert it. Just understand where the wish to change this came from since it can reveal further issues (like that the API contract is fragile to begin with). |
This is unobservable. There should be no need to repeatedly clear it if we already did that once.